sequenceDiagram
    participant Caller as W3DView / InGameUI
    participant Disp as Display (base)
    participant W3D as W3DDisplay
    participant R2D as Render2DClass

    Caller->>Disp: beginBatch()
    Disp->>W3D: onBeginBatch()
    W3D->>R2D: Reset()
    note over W3D: m_batchNeedsInit=TRUE<br/>m_batchTexture=nullptr

    loop Each draw* call
        Caller->>W3D: drawImage / drawLine / ...
        W3D->>W3D: setup2DRenderState(tex, mode, grayscale)
        alt Same state as current batch
            W3D-->>W3D: early-return (no flush)
        else State changed
            W3D->>W3D: onFlush()
            W3D->>R2D: Render() + Reset()
            W3D->>W3D: swap tex ref, update mode/grayscale
        end
        W3D->>R2D: Add_Quad / Add_Line / Add_Tri
    end

    Caller->>Disp: endBatch()
    Disp->>W3D: onFlush()
    W3D->>R2D: Render() + Reset()
    Disp->>W3D: onEndBatch()
    W3D->>W3D: REF_PTR_RELEASE(m_batchTexture)

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Line: 2739

Comment:
**Long clipping condition on one line**

This is a very long line (~125 chars) that mixes all four boundary checks together. In the early-out block at the top of the function the same four conditions are already split across four indented lines — the inner clipping check would benefit from the same treatment for consistency and debuggability (each sub-condition can have its own breakpoint).

```suggestion
		if (screen_rect.Left   < m_clipRegion.lo.x ||
			screen_rect.Right  > m_clipRegion.hi.x ||
			screen_rect.Top    < m_clipRegion.lo.y ||
			screen_rect.Bottom > m_clipRegion.hi.y)
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Line: 2630

Comment:
**Long clipping condition on one line (Generals mirror)**

Same long single-line clipping check as in the GeneralsMD copy — worth splitting onto four lines for consistency with the early-out block directly above it.

```suggestion
		if (screen_rect.Left   < m_clipRegion.lo.x ||
			screen_rect.Right  > m_clipRegion.hi.x ||
			screen_rect.Top    < m_clipRegion.lo.y ||
			screen_rect.Bottom > m_clipRegion.hi.y)
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (21): Last reviewed commit: "implemented feedback" | Re-trigger Greptile}

Significantly improves #109
Fully fixing this requires significant refactoring to Render2DSentenceClass for fonts

@greptileai

"One P1 render-state initialization bug in onBeginBatch() should be fixed before merging to avoid potential cross-frame visual corruption."

The P1 finding could be a false positive based on a local variable analysis that omits the downstream hardware synchronization in Render2DClass::Render() (render2d.cpp:601).

Render2DClass is a self-synchronizing state machine. Its Render() call (line 628 for textures, line 694 for shaders) explicitly pushes its internal members to the DX8Wrapper on every execution. The tracking variables in W3DDisplay (m_batchTexture, etc.) are mirror-images of Render2DClass's internal members.

happy?

No, the P1 finding stands.

Your argument about Render2DClass being self-synchronizing addresses hardware state consistency, but not the C++ undefined behavior in W3DDisplay.

The issue:

• onFlush() reads !m_batchNeedsInit (line 617 of your diff)
• setup2DRenderState() reads !m_batchNeedsInit (line 627)
• If uninitialized, these branches are unpredictable

Even if Render2DClass correctly synchronizes hardware state on every Render() call, W3DDisplay's control flow still depends on m_batchNeedsInit to decide:

1. Whether to call Render() in onFlush()
2. Whether to skip redundant state setup in setup2DRenderState()

An uninitialized bool means these decisions are based on garbage. The fact that the hardware might recover downstream doesn't make reading uninitialized memory defined behavior.

The fix is one line. Keep it.

I see greptile woke up on the sassy side of the motherboard.

Reduces draw calls by batching 2D elements by texture state in setup2DRenderState.

Environment Scenario (-quickstart) Before After
Main Machine Skirmish Screen 477 FPS 918 FPS
M1 Macbook (Wine) Skirmish Screen 16 FPS 61 FPS
M1 Macbook (Wine) In-Game (Match Start) 22 FPS 33 FPS

Another test would be to get a lot of units on screen then select them all so their unit info is being drawn.

The health bars make use of the line drawing classes which i believe are inherently affected by this change.

I was initially going to say that health bars render in drawViews() (outside this batch), but your comment made me realize it's only a 6-line change to wrap that too.

Jumped from 45 FPS to 56 FPS.

I was initially going to say that health bars render in drawViews() (outside this batch), but your comment made me realize it's only a 6-line change to wrap that too.

Jumped from 45 FPS to 56 FPS.

Nice ~30% improvement there

Compile errors need to be fixed before this can be reviewed

Interestingly the replays are failing

The replays look like an CI issue.

My profiler is currently crashing, so I can't give you a function-level breakdown of the 2D render path alone.

Best I have for now is indications:
The Skirmish screen showing 918 fps instead of 477 fps when using -quickstart.
When in a match with the 3d viewport disabled it jumped from 2200-3300 fps to 2700-3700 fps.
Selecting 1,000 hackers caused an immediate drop from 56 FPS to 45 FPS, with this PR, the stays near the same even with all 1,000 units selected.